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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 6, 2020, 19:18 UTC
Message-ID: <xmqqimaifg5m.fsf@gitster.c.googlers.com>
In-Reply-To: <a0513d6f-1f69-683d-d6c5-75b17b8b6890@theori.io>

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.