From: Jinoh Kang Date: Tue, 10 Nov 2020 13:16:56 GMT Subject: Re: [PATCH v3] diff: make diff_free_filespec_data accept NULL Message-ID: <17491ac3-ce1b-ca5d-d4e8-9fa8ab72d7e3@theori.io> In-Reply-To: On 11/10/20 12:08 PM, Johannes Schindelin wrote: > Hi Jinoh, > > On Fri, 6 Nov 2020, Jinoh Kang wrote: > >> Commit 3aef54e8b8 ("diff: munmap() file contents before running external >> diff") introduced calls to diff_free_filespec_data in >> run_external_diff, which may pass NULL pointers. > > Sorry for the breakage! No worries. Those functions were indeed confusing... > > Maybe the commit message could talk a bit about the circumstances when > this happens? Of course, I (and every other reader...) could analyze your > patch to guess what it is that triggers the bug, but it's really a good > use of the commit message to describe it in plain English. Agreed. The v1 PATCH, which was off-list, did explain that NULL filespecs are used to indicate unmerged files (i.e. with unresolved conflicts). > >> >> Fix this and prevent any such bugs in the future by making >> `diff_free_filespec_data(NULL)` a no-op. >> >> Fixes: 3aef54e8b8 ("diff: munmap() file contents before running external diff") > > I am unaware of any other commit having a `Fixes:` trailer in the commit > message. In any case, I would have expected `Fixes:` to mention a ticket > or a bug report, not the commit that fixed _a separate_ issue (but > unfortunately introduced this regression). Sorry for this one; somehow I didn't notice that git and linux use different conventions. -- Jinoh Kang Theori