Hi Junio, On Tue, 8 Oct 2019, Junio C Hamano wrote: > Thomas Gummerer <t.gummerer@xxxxxxxxx> writes: > > > We can however rely on 'patch.def_name' in that case, which is > > extracted from the 'diff --git' line and should be equal to > > 'patch.new_name'. Use that instead to avoid the segfault. > > This patch makes the way this function calls parse_git_diff_header() > more in line with the way how it is used by its original caller in > apply.c::find_header(), but not quite. > > I have to wonder if we want to move a bit of code around so that > callers of parse_git_diff_header() do not have to worry about > def_name and can rely on new_name and old_name fields correctly > filled. > > There was only one caller of the parse_git_diff_header() function > before range-diff. The division of labour between find_header() and > parse_git_diff_header() did not make any difference to the consumers > of the new/old_name fields. They only cared that they do not have > to worry about def_name. But by calling parse_git_diff_header() > that forces the caller to worry about def_name (which is done by > find_header() to free its callers from doing so), range-diff took > responsibility of caring, which was suboptimal. The interface could > have been a bit more cleaned up before we started to reuse it in the > new caller, and as this bug shows, it may be time to do so now, no? > > Perhaps before returing, parse_git_diff_header() should fill the two > names with xstrdup() of def_name if (!old_name && !new_name && > !!def_name); all other cases the existing caller and this new caller > would work unchanged correctly, no? FWIW I totally agree. Ciao, Dscho
